Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix OOM (Out Of Memory) Errors #138

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix OOM (Out Of Memory) Errors #138

wants to merge 1 commit into from

Conversation

mjnaderi
Copy link

Summary

This PR fixes mattermost/mattermost#20625.

Ticket Link

None

@hanzei hanzei requested a review from mvitale1989 December 4, 2023 19:33
@hanzei hanzei added the 2: Dev Review Requires review by a core committer label Dec 4, 2023
@mvitale1989
Copy link
Member

Thank you for your contribution @mjnaderi !
I don't have an objection to this change myself, but I don't think this fixes mattermost/mattermost#20625 , since the reporter mentions using Openshift, so they are unlikely to be using docker compose.
I would include @agnivade as a reviewer, to double check if there are conditions under which such a spike in PIDs in the Mattermost container is expected.

@mvitale1989 mvitale1989 requested a review from agnivade December 4, 2023 20:23
mvitale1989
mvitale1989 previously approved these changes Dec 4, 2023
View reviewed changes
agnivade
agnivade reviewed Dec 5, 2023
View reviewed changes
docker-compose.yml
@@ -8,7 +8,7 @@ services:
restart: ${RESTART_POLICY}
security_opt:
- no-new-privileges:true
pids_limit: 100
pids_limit: 200
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no need to change the limit for postgres.

docker-compose.yml
@@ -31,7 +31,7 @@ services:
restart: ${RESTART_POLICY}
security_opt:
- no-new-privileges:true
pids_limit: 200
pids_limit: 400
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

More than 200 processes means that something is very wrong with the system. I'd like to get a list of the processes if possible and see if there's an actual bug from the product that needs to be fixed. Otherwise, there's a chance that we are hiding something under the rug here.

@mvitale1989
Copy link
Member

Thanks for the input Agniva! Agreed, if more than 200 PIDs is an unreasonable number then we'd better understand what that is first before thinking about merging this change.
I'll remove my approval for the time being to reduce confusion

@mvitale1989 mvitale1989 dismissed their stale review December 5, 2023 12:46

We first need to understand why there is a spike in PIDs

@erfantkerfan
Copy link
Contributor

I had the same problem and changing this values fixed it.
Like you said, I pushed the problem under the rug by increasing the limit.

@agnivade
Copy link
Member

Did you try listing out the processes to see what was wrong?

@erfantkerfan
Copy link
Contributor

Did you try listing out the processes to see what was wrong?

No, but since then, I did not have any crashes, so I did not have the chance to

@hanzei hanzei added the Awaiting Submitter Action Blocked on the author label Dec 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@agnivade agnivade agnivade left review comments

@mvitale1989 mvitale1989 mvitale1989 left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
2: Dev Review Requires review by a core committer Awaiting Submitter Action Blocked on the author
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Mattermost suddenly goes out of memory (OOM) and reboots
5 participants
@mjnaderi @mvitale1989 @erfantkerfan @agnivade @hanzei